Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: automatic type-only imports #7382

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

demensky
Copy link
Contributor

@demensky demensky commented Nov 21, 2023

Automatically add type to import/export if the symbol is used only for type annotations and declarations.

Fixes: https://x.com/BenLesh/status/1724808188988633490

@demensky demensky marked this pull request as ready for review November 21, 2023 19:17
@jakovljevic-mladen
Copy link
Member

Hey @demensky, we've already had similar PRs in past and unfortunately, they didn't work for us.

Please take a look at this PR #6943 where you can find why it didn't work for us.

Did you maybe try to run the docs app and check whether the issue explained in the mentioned PR applies to the changes form this PR?

@jakovljevic-mladen
Copy link
Member

Hehe, have I now completely missed the point?

It looks like export type creates an issue for us, but this PR is doing the import type 😄

However, I'd still like to know whether the docs app has issues with these changes.

@demensky
Copy link
Contributor Author

@jakovljevic-mladen, I checked the docs app and didn't find any problems with it.

Thanks to you, I also decided to add export type, checked it again and still found no problems. Apparently the problem is no longer relevant.

Example

P.S. I deleted apps/rxjs.dev/src/generated before starting the docs app build.

@jakovljevic-mladen
Copy link
Member

Actually, I don't think we had an issue with ShareConfig page being rendered, but with links that point to the ShareConfig page or any other exported type page that we have.

So, to test it, please open config page locally and verify that link from this line works and that it points to GlobalConfig page.

@demensky
Copy link
Contributor Author

@jakovljevic-mladen. Here's video proof. Although I would be very grateful if you also checked locally.

2023-11-22.14.54.59.webm

@jakovljevic-mladen
Copy link
Member

Cool, I think that should be good enough. I also checked pipelines and it seems that we don't have issues there as well, so I think it's fine.

packages/rxjs/spec-dtslint/Observable-spec.ts Outdated Show resolved Hide resolved
"rules": {
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/no-unused-vars": "off",
"@typescript-eslint/no-unsafe-declaration-merging": "off",
"@typescript-eslint/consistent-type-imports": "warn",
"@typescript-eslint/consistent-type-exports": "warn",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should these be "error" instead of warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7391. I also discovered that eslint also checks dist. I will create a separate PR.

@benlesh
Copy link
Member

benlesh commented Nov 28, 2023

In general, I think this is fine. It doesn't really "fix" my ask, which is I legit wish that VS Code would highlight types differently... but besides that, I think it's a solid add. It makes the code easier to grep, if maybe mildly annoying to refactor.

We might want to make the eslint rule an error if we want to keep it clean.

@benlesh benlesh merged commit e19c7ef into ReactiveX:master Nov 28, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants